Reimplement std's thread parker on top of events on SGX #98391

joboet · 2022-06-22T15:54:36Z

Mutex and Condvar are being replaced by more efficient implementations, which need thread parking themselves (see #93740). Therefore, the generic Parker needs to be replaced on all platforms where the new lock implementation will be used.

SGX enclaves have a per-thread event state, which allows waiting for and setting specific bits. This is already used by the current mutex implementation. The thread parker can however be much more efficient, as it only needs to store the TCS address of one thread. This address is stored in a state variable, which can also be set to indicate the thread was already notified.

park_timeout does not guard against spurious wakeups like the current condition variable does. This is allowed by the API of Parker, and I think it is better to let users handle these wakeups themselves as the guarding is quite expensive and might not be necessary.

@jethrogb as you wrote the initial SGX support for std, I assume you are the target maintainer? Could you help me test this, please? Lacking a x86_64 chip, I can't run SGX.

rust-highfive · 2022-06-22T15:54:39Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-22T15:54:41Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

jethrogb · 2022-06-22T16:09:42Z

Cool stuff. It will take use some time to review this to ensure security. cc @mzohreva @raoulstrackx

Mark-Simulacrum · 2022-06-26T16:30:49Z

Once you have reviewed, please re-assign back to me or someone else for the final r+

library/std/src/sys/sgx/thread_parker.rs

rustbot · 2022-09-05T08:19:24Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

408: Document `TCS` struct limitations r=jethrogb a=raoulstrackx Document that a TCS struct should never be located as the beginning of an enclave. (ref rust-lang/rust#98391) 409: Fix broken link to std library r=jethrogb a=raoulstrackx Co-authored-by: Raoul Strackx <raoul.strackx@fortanix.com>

408: Document `TCS` struct limitations r=jethrogb a=raoulstrackx Document that a TCS struct should never be located as the beginning of an enclave. (ref rust-lang/rust#98391) Co-authored-by: Raoul Strackx <raoul.strackx@fortanix.com>

raoulstrackx · 2022-10-12T12:02:33Z

bors r+

joboet · 2022-10-12T12:11:45Z

Thank you for the review!

r? @Mark-Simulacrum

Mark-Simulacrum · 2022-10-16T15:04:01Z

r? @m-ou-se

m-ou-se · 2022-12-08T17:08:43Z

@bors r+

bors · 2022-12-08T17:08:45Z

📌 Commit a40d300 has been approved by m-ou-se

It is now in the queue for this repository.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#98391 (Reimplement std's thread parker on top of events on SGX) - rust-lang#104019 (Compute generator sizes with `-Zprint_type_sizes`) - rust-lang#104512 (Set `download-ci-llvm = "if-available"` by default when `channel = dev`) - rust-lang#104901 (Implement masking in FileType comparison on Unix) - rust-lang#105082 (Fix Async Generator ABI) - rust-lang#105109 (Add LLVM KCFI support to the Rust compiler) - rust-lang#105505 (Don't warn about unused parens when they are used by yeet expr) - rust-lang#105514 (Introduce `Span::is_visible`) - rust-lang#105516 (Update cargo) - rust-lang#105522 (Remove wrong note for short circuiting operators) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…ou-se Replace generic thread parker with explicit no-op parker With rust-lang#98391 merged, all platforms supporting threads now have their own parking implementations. Therefore, the generic implementation can be removed. On the remaining platforms (really just WASM without atomics), parking is not supported, so calls to `thread::park` now return instantly, which is [allowed by their API](https://doc.rust-lang.org/nightly/std/thread/fn.park.html). This is a change in behaviour, as spurious wakeups do not currently occur since all platforms guard against them. It is invalid to depend on this, but I'm still going to tag this as libs-api for confirmation. `@rustbot` label +T-libs +T-libs-api +A-atomic r? rust-lang/libs

…ou-se Replace generic thread parker with explicit no-op parker With rust-lang#98391 merged, all platforms supporting threads now have their own parking implementations. Therefore, the generic implementation can be removed. On the remaining platforms (really just WASM without atomics), parking is not supported, so calls to `thread::park` now return instantly, which is [allowed by their API](https://doc.rust-lang.org/nightly/std/thread/fn.park.html). This is a change in behaviour, as spurious wakeups do not currently occur since all platforms guard against them. It is invalid to depend on this, but I'm still going to tag this as libs-api for confirmation. ``@rustbot`` label +T-libs +T-libs-api +A-atomic r? rust-lang/libs

…ou-se Replace generic thread parker with explicit no-op parker With rust-lang#98391 merged, all platforms supporting threads now have their own parking implementations. Therefore, the generic implementation can be removed. On the remaining platforms (really just WASM without atomics), parking is not supported, so calls to `thread::park` now return instantly, which is [allowed by their API](https://doc.rust-lang.org/nightly/std/thread/fn.park.html). This is a change in behaviour, as spurious wakeups do not currently occur since all platforms guard against them. It is invalid to depend on this, but I'm still going to tag this as libs-api for confirmation. ```@rustbot``` label +T-libs +T-libs-api +A-atomic r? rust-lang/libs

…ou-se Replace generic thread parker with explicit no-op parker With rust-lang#98391 merged, all platforms supporting threads now have their own parking implementations. Therefore, the generic implementation can be removed. On the remaining platforms (really just WASM without atomics), parking is not supported, so calls to `thread::park` now return instantly, which is [allowed by their API](https://doc.rust-lang.org/nightly/std/thread/fn.park.html). This is a change in behaviour, as spurious wakeups do not currently occur since all platforms guard against them. It is invalid to depend on this, but I'm still going to tag this as libs-api for confirmation. ````@rustbot```` label +T-libs +T-libs-api +A-atomic r? rust-lang/libs

joboet added 2 commits June 22, 2022 16:42

std: rewrite SGX thread parker

9678cec

std: reimplement SGX thread joining to use Parker

633d46d

rust-highfive assigned Mark-Simulacrum Jun 22, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 22, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022

joboet mentioned this pull request Jun 26, 2022

Tracking issue for improving std::sync::{Mutex, RwLock, Condvar} #93740

Closed

63 tasks

Mark-Simulacrum assigned jethrogb and unassigned Mark-Simulacrum Jun 26, 2022

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022

raoulstrackx reviewed Aug 29, 2022

View reviewed changes

library/std/src/sys/sgx/thread_parker.rs Outdated Show resolved Hide resolved

library/std/src/sys/sgx/thread_parker.rs Show resolved Hide resolved

library/std/src/sys/sgx/thread_parker.rs Show resolved Hide resolved

library/std/src/sys/sgx/thread_parker.rs Show resolved Hide resolved

std: clarify semantics of SGX parker

a40d300

jethrogb mentioned this pull request Sep 5, 2022

Tcs should not be a NonNull pointer fortanix/rust-sgx#407

Closed

raoulstrackx mentioned this pull request Sep 5, 2022

Document TCS struct limitations fortanix/rust-sgx#408

Merged

rust-highfive assigned Mark-Simulacrum and unassigned jethrogb Oct 12, 2022

rust-highfive assigned m-ou-se and unassigned Mark-Simulacrum Oct 16, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2022

matthiaskrgr mentioned this pull request Dec 10, 2022

Rollup of 10 pull requests #105525

Merged

bors merged commit ae8794c into rust-lang:master Dec 10, 2022

rustbot added this to the 1.68.0 milestone Dec 10, 2022

joboet mentioned this pull request Dec 14, 2022

Replace generic thread parker with explicit no-op parker #105695

Merged

joboet deleted the sgx_parker branch December 22, 2022 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement std's thread parker on top of events on SGX #98391

Reimplement std's thread parker on top of events on SGX #98391

joboet commented Jun 22, 2022 •

edited

Loading

rust-highfive commented Jun 22, 2022

rust-highfive commented Jun 22, 2022

jethrogb commented Jun 22, 2022

Mark-Simulacrum commented Jun 26, 2022

rustbot commented Sep 5, 2022

raoulstrackx commented Oct 12, 2022

joboet commented Oct 12, 2022

Mark-Simulacrum commented Oct 16, 2022

m-ou-se commented Dec 8, 2022

bors commented Dec 8, 2022

Reimplement std's thread parker on top of events on SGX #98391

Reimplement std's thread parker on top of events on SGX #98391

Conversation

joboet commented Jun 22, 2022 • edited Loading

rust-highfive commented Jun 22, 2022

rust-highfive commented Jun 22, 2022

jethrogb commented Jun 22, 2022

Mark-Simulacrum commented Jun 26, 2022

rustbot commented Sep 5, 2022

raoulstrackx commented Oct 12, 2022

joboet commented Oct 12, 2022

Mark-Simulacrum commented Oct 16, 2022

m-ou-se commented Dec 8, 2022

bors commented Dec 8, 2022

joboet commented Jun 22, 2022 •

edited

Loading